perf: replace RLock with Lock where re-entrant locking is not needed (~11ns saving, -14%)#796
perf: replace RLock with Lock where re-entrant locking is not needed (~11ns saving, -14%)#796mykaul wants to merge 2 commits intoscylladb:masterfrom
Conversation
fbb04b2 to
5f8a314
Compare
V2 ChangesFixed: deadlock in
Fix: Restructured Additional cleanup:
Tests: 683 unit tests pass (660 core + 23 IO), 0 failures. |
Follow-up: Skip lock acquisition when no orphaned requests in process_msgCommit: 2e5a6c6 What changedIn Now we check Thread safetyThe unlocked truthiness check on a
Benchmark results (Python 3.14, 2M iterations)
Testing
|
c8f4db2 to
2e5a6c6
Compare
Convert 7 of 8 RLock instances to plain Lock. All verified to use only flat (non-recursive) acquisition patterns: - Connection.lock (hot path: every message send/receive) - Cluster._lock (connect/shutdown) - ControlConnection._lock and _reconnection_lock - Metadata._hosts_lock and TokenMap._rebuild_lock - Host.lock and cqlengine Connection.lazy_connect_lock Session._lock is kept as RLock because run_add_or_renew_pool() uses manual release/acquire inside a 'with' block. Benchmark: RLock 'with' stmt is ~14% slower than plain Lock.
Check orphaned_request_ids truthiness before acquiring the lock. Since orphaned requests are rare (only on timeouts), the set is almost always empty. Skipping the lock in the common case saves ~57 ns per response. The unlocked truthiness check on a set is thread-safe under the GIL. Worst case (false positive): we enter the lock block and re-check, which is correct. Worst case (false negative): an orphaned response is processed normally — acceptable behavior. Benchmark (2M iters, Python 3.14): Empty set (common): 80.6 -> 23.2 ns (3.47x, -57.4 ns/response) Non-empty set (rare): 79.7 -> 87.8 ns (+8.1 ns overhead)
2e5a6c6 to
014e82e
Compare
Summary
Convert 7 of 8 RLock instances to plain Lock. All verified to use only flat (non-recursive) acquisition patterns:
Connection.lockCluster._lockControlConnection._lockControlConnection._reconnection_lockMetadata._hosts_lockTokenMap._rebuild_lockHost.lockcqlengine.Connection.lazy_connect_lockSession._lockis kept as RLock becauserun_add_or_renew_pool()uses manualrelease()/acquire()inside awithblock, which requires re-entrant semantics.Benchmark
withstmt)RLockLockTests
test_update_host_sequential_lockspecifically validates thatMetadata.update_host()works with plain Lock (sequential, not nested acquisition)